-
Notifications
You must be signed in to change notification settings - Fork 129
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
VACMS 20241 - autosuggest/typeahead/combobox address for facility locator #34361
base: main
Are you sure you want to change the base?
Conversation
….com:department-of-veterans-affairs/vets-website into VACMS-20241-autosuggest-valid-address-mapbox
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some observations:
- If I select an option from the dropdown, then go back and edit it, ignoring the dropdown options, the search term in the result string is not updated.
Screen.Recording.2025-02-05.at.9.45.39.AM.mov
- If I've already selected an option from the dropdown, then I click "Use my location," nothing happens.
Screen.Recording.2025-02-05.at.9.51.24.AM.mov
- Similarly, if I have an option from the dropdown, then clear that option and click "Use my location," nothing happens. I can see a geocoding call in the network tab happening, so perhaps it's having trouble replacing the input value?
Screen.Recording.2025-02-05.at.9.56.09.AM.mov
- This is likely out-of-scope because it's also a thing in prod, but I'm curious: are we not wanting to enforce that people choose a city when searching? Noticing I can search for "Oklahoma" in both this local build and prod.
src/applications/facility-locator/components/AddressAutosuggest.jsx
Outdated
Show resolved
Hide resolved
src/applications/facility-locator/components/AddressAutosuggest.jsx
Outdated
Show resolved
Hide resolved
src/applications/facility-locator/components/AddressAutosuggest.jsx
Outdated
Show resolved
Hide resolved
return ( | ||
<span className="usa-input-error-message" role="alert"> | ||
<span className="sr-only">Error</span> | ||
Please fill in a city, state, or postal code. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We stopped using Please
in other apps per DS recommendations; should we be using it here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's being updated in the progressive disclosure ticket. We're also changing the string associated with it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can update here to be in line with that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to update it here to be in line with DS standards (if we know what it should be).
src/applications/facility-locator/components/Autosuggest/AutosuggestOption.jsx
Outdated
Show resolved
Hide resolved
src/applications/facility-locator/components/Autosuggest/StateReducer.js
Outdated
Show resolved
Hide resolved
src/applications/facility-locator/components/Autosuggest/sass/autosuggest.scss
Outdated
Show resolved
Hide resolved
proximity: 'ip', | ||
}) | ||
.send(); | ||
// TODO: display error message if geolocation fails? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this still need to be handled before we can merge?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an old issue and possible a zero silent failures concern. I removed, but we probably need to handle. It's direct with mapbox that the issue may happen.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is handling this in scope for this ticket? If not, could we spin up a new ticket for this and add the ticket # as part of this comment please?
src/applications/facility-locator/tests/components/AutosuggestTestComponent.jsx
Outdated
Show resolved
Hide resolved
src/applications/facility-locator/tests/components/Autosuggest.unit.spec.jsx
Show resolved
Hide resolved
….com:department-of-veterans-affairs/vets-website into VACMS-20241-autosuggest-valid-address-mapbox
src/applications/facility-locator/components/Autosuggest/StateReducer.js
Outdated
Show resolved
Hide resolved
src/applications/facility-locator/components/AddressAutosuggest.jsx
Outdated
Show resolved
Hide resolved
src/applications/facility-locator/components/AddressAutosuggest.jsx
Outdated
Show resolved
Hide resolved
src/applications/facility-locator/components/AddressAutosuggest.jsx
Outdated
Show resolved
Hide resolved
return ( | ||
<span className="usa-input-error-message" role="alert"> | ||
<span className="sr-only">Error</span> | ||
Please fill in a city, state, or postal code. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's being updated in the progressive disclosure ticket. We're also changing the string associated with it.
src/applications/facility-locator/components/Autosuggest/AutosuggestOption.jsx
Outdated
Show resolved
Hide resolved
return ( | ||
<span className="usa-input-error-message" role="alert"> | ||
<span className="sr-only">Error</span> | ||
Please fill in a city, state, or postal code. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can update here to be in line with that.
src/applications/facility-locator/components/AddressAutosuggest.jsx
Outdated
Show resolved
Hide resolved
src/applications/facility-locator/components/AddressAutosuggest.jsx
Outdated
Show resolved
Hide resolved
src/applications/facility-locator/components/Autosuggest/StateReducer.js
Outdated
Show resolved
Hide resolved
src/applications/facility-locator/components/Autosuggest/index.jsx
Outdated
Show resolved
Hide resolved
If the component allows a free-text response, should it be doing its "best guess" matching against responses that don't quite make sense? A couple of examples where I typed some junk and it searched for one of the suggestions that wasn't selected: Screen.Recording.2025-02-06.at.6.44.19.AM.movScreen.Recording.2025-02-06.at.6.41.39.AM.mov |
…arching will be the same as what is searched when typed
So the reason we weren't getting the search terms in the AutosuggestOptions was because I was testing without country in that box but it was still searching country with the search button. Now they show the same thing, but both behaviors are what are seen in production. Mapbox doesn't understand misspelled |
@eselkin Not sure I understand. I think the root of my question is: if you type something in the search box and do not select any of the dropdown options, shouldn't it search for exactly what you typed rather than trying to pick a suggestion? |
That is the effect. It is searching for what you typed. Unfortunately the search comes back with what mapbox says it thinks you meant. |
I finished functional testing on my end - just a couple comments left in the diffs still. It's hard to wrap my brain around whether this will work 100% for VA health services autosuggest at this stage; I think we'll find out when we get there. |
….com:department-of-veterans-affairs/vets-website into VACMS-20241-autosuggest-valid-address-mapbox
Are you removing, renaming or moving a folder in this PR?
Did you change site-wide styles, platform utilities or other infrastructure?
Summary
facilities_use_address_typeahead
Related issue(s)
PR for flipper
department-of-veterans-affairs/vets-api#20516
Testing done
Screenshots
New Autosuggest for find-locations
https://github.com/user-attachments/assets/7ac0ad4f-1b21-4748-b2b9-256b8d5136de
What areas of the site does it impact?
Affects the zip code/city,state field of the Facility Locator
Acceptance criteria
Provides up to 5 mapbox found locations for a user input.
Quality Assurance & Testing
Error Handling
Authentication
Requested Feedback
Check /find-locations